Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement mismatched_target_os lint #5506

Merged
merged 5 commits into from
Apr 27, 2020
Merged

Conversation

ebroto
Copy link
Member

@ebroto ebroto commented Apr 22, 2020

I've extended the check suggested in the issue to all the currently supported operating systems instead of limiting it to linux and macos, let me know if we want to do this.

Also, I've restored the text There are over XXX lints ... in the README as it was matched against by cargo dev new_lint.

changelog: Added mismatched_target_os lint to warn when an operating system is used in target family position in a #[cfg] attribute

Closes #3949

clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Apr 25, 2020
@ebroto ebroto force-pushed the mismatched_target_os branch from dfa13ab to be57434 Compare April 25, 2020 19:30
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! This looks really good. One last thing

clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
@ebroto ebroto force-pushed the mismatched_target_os branch from be57434 to da6a5d3 Compare April 25, 2020 21:53
@ebroto
Copy link
Member Author

ebroto commented Apr 25, 2020

Oh, I was not aware of the stderr line limit. I guess I can remove some OS from the unix group as all of them are in the same slice in the end...

@flip1995
Copy link
Member

I would suggest to keep tests for every OS, but split up the tests in unix and non-unix. This should meet the line limit of 200.

ebroto added 5 commits April 26, 2020 21:27
- Show just one error message with multiple suggestions in case of
  using multiple times an OS in target family position
- Only suggest #[cfg(unix)] when the OS is in the Unix family
- Test all the operating systems
@ebroto ebroto force-pushed the mismatched_target_os branch from 67da32e to 303e7d1 Compare April 26, 2020 19:36
@ebroto
Copy link
Member Author

ebroto commented Apr 26, 2020

I would suggest to keep tests for every OS, but split up the tests in unix and non-unix. This should meet the line limit of 200.

Yeah, that's way better :)

Thanks for this and all your other suggestions!

@flip1995
Copy link
Member

@bors r+

Thanks! Great lint.

@bors
Copy link
Contributor

bors commented Apr 27, 2020

📌 Commit 303e7d1 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Apr 27, 2020

⌛ Testing commit 303e7d1 with merge 5d8a3e8...

@bors
Copy link
Contributor

bors commented Apr 27, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 5d8a3e8 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint against #[cfg(linux)] → #[cfg(unix)]
4 participants